Limit User Input Modal to One Window#1730
Conversation
This determines the correct block id and sends the modal only to the window that contains the block that made the initial request.
WalkthroughThis pull request introduces several interconnected changes across multiple files in the frontend and backend of the application. The modifications primarily focus on enhancing context management, particularly around user input and connection handling. The changes involve the removal of debugging Additionally, server-side methods in 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/app/store/global.ts (1)
192-192: Consider using debug level logging.The console.log for user input events might be too verbose for production. Consider using a debug level logger or making it conditional based on development mode.
- console.log("userinput event handler", event); + if (isDev()) { + console.log("userinput event handler", event); + }pkg/wshrpc/wshserver/wshserver.go (2)
685-686: LGTM with a suggestion for improvement.The context enrichment is properly implemented. Consider extracting the common context enrichment pattern into a helper function since it's used in multiple methods.
+func enrichConnContext(ctx context.Context, blockId string) context.Context { + ctx = genconn.ContextWithConnData(ctx, blockId) + return termCtxWithLogBlockId(ctx, blockId) +}This would reduce code duplication and make future modifications easier.
Line range hint
242-686: Well-structured context enrichment implementation.The addition of connection data to the context across these methods provides a clean way to track and manage connection-related state. This implementation aligns well with the PR's objective of limiting user input modal to one window by ensuring proper context propagation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/modals/userinputmodal.tsx(0 hunks)frontend/app/store/global.ts(2 hunks)frontend/wave.ts(1 hunks)pkg/genconn/genconn.go(1 hunks)pkg/userinput/userinput.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(5 hunks)
💤 Files with no reviewable changes (1)
- frontend/app/modals/userinputmodal.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
pkg/userinput/userinput.go (2)
66-71: LGTM: Window-scoped event publishing.The modification to include
Scopes: []string{windowId}ensures the user input request is only sent to the specific window that initiated it.
81-94: Verify error handling in the window ID resolution chain.The window ID resolution logic looks correct but has multiple database lookups that could fail. Consider:
- Adding debug logging for troubleshooting
- Handling edge cases where any of the IDs might be empty strings
Let's verify the error handling:
✅ Verification successful
Error handling and logging are already robust in the window ID resolution chain.
The code has:
- Proper error handling with descriptive messages
- Debug logging for database operations and error cases
- Implicit empty string validation through the database layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential nil checks and error handling patterns in similar code rg -A 2 "DBFind.*Id" | grep -B 2 "if.*err.*!= nil"Length of output: 2506
Script:
#!/bin/bash # Check DB function implementations ast-grep --pattern 'func DBFind$_(ctx context.Context, id string)' # Check for logging patterns rg "log\." -A 2Length of output: 53993
pkg/genconn/genconn.go (2)
18-24: LGTM: Clean context key implementation.Good use of a private struct type for the context key to prevent key collisions.
26-42: LGTM: Robust context data management.The implementation follows best practices:
- Proper nil checks
- Immutable context handling
- Type assertion with proper error handling
frontend/wave.ts (1)
166-166: LGTM: Proper initialization options forwarding.Correctly passes the window context to the event subscription initialization.
frontend/app/store/global.ts (1)
171-171: LGTM: Window-scoped event subscription.The changes correctly implement window-scoped user input handling:
- Function now accepts initialization options
- User input events are properly scoped to the specific window
Also applies to: 196-196
pkg/wshrpc/wshserver/wshserver.go (4)
24-24: LGTM!The
genconnpackage import is correctly placed and used consistently throughout the file.
242-244: LGTM!The context enrichment with connection data is properly implemented, maintaining a logical order of operations.
632-633: LGTM!The context enrichment is properly implemented, preserving the existing connection handling logic.
662-663: LGTM!The context enrichment is properly implemented, maintaining compatibility with both WSL and regular connections.
When a connection request is made from a block, only ask for user input in the window that made the request.
1 participant
When a connection request is made from a block, only ask for user input in the window that made the request.